-
Notifications
You must be signed in to change notification settings - Fork 134
Ai sdk 5 migration #10903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Ai sdk 5 migration #10903
Conversation
|
E2E Tests 🚀 |
sharon-wang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far with Amazon Bedrock, OpenAI and OpenRouter via Custom Provider 👍 getPlot tool is working with Amazon Bedrock, so I wonder if our plot image handling is compatible with Anthropic models but not the OpenAI models?
Providers that don't support the responses endpoint aren't working though -- I think we'll need some way to override the chat endpoint for those providers to /chat/completions
| * AI SDK 5 supports images in tool results via the 'content' output type with 'media' parts. | ||
| */ | ||
| function getPlotToolResultToAiMessage(part: vscode.LanguageModelToolResultPart2): ai.CoreUserMessage { | ||
| function getPlotToolResultToAiMessage(part: vscode.LanguageModelToolResultPart2): ai.ToolModelMessage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering: is this function still needed? I think that convertToolResultToAiMessageExperimentalContent should be able to handle tool results with images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it and tested the change. Various models and providers were able to handle the tool output without this function. It also fixed the problem with GPT models on Snowflake where it couldn't use the get plot tool output.
| if (cacheBreakpoint && bedrockCacheBreakpoint) { | ||
| cacheBreakpoint = false; | ||
| markBedrockCacheBreakpoint(toolMessage); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably doesn't have to be addressed just yet, but if/when Anthropic goes through this code path (instead of the separate Anthropic provider code), then we'll want to support their cache breakpoint markers as well.
| // Fix empty role field | ||
| if (choice.delta.role === '') { | ||
| choice.delta.role = 'assistant'; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check here because of Snowflake, or other providers that don't quite adhere to the OpenAI completions format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this was originally added for Snowflake, but it's possible other providers may omit this as well
| } | ||
| } | ||
|
|
||
| transformedLines.push(`data: ${JSON.stringify(data)}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the types from OpenAI, I suggest actually extracting a lot of the code above into a function which takes as input something that's close to a ChatCompletionChunk, but where the type takes into account the possibly-missing fields. The function would return something that actually is a ChatCompletionChunk.
Then you can really lean into type checking to make sure all the cases are taken care of.
For example, if you define this type, then the role could be "":
type PossiblyBrokenChatCompletionChunk = Omit<OpenAI.ChatCompletionChunk, 'choices'> & {
choices: Array<Omit<OpenAI.ChatCompletionChunk['choices'][0], 'delta'> & {
delta: Omit<OpenAI.ChatCompletionChunk['choices'][0]['delta'], 'role'> & {
role?: OpenAI.ChatCompletionChunk['choices'][0]['delta']['role'] | ''
}
}>
};I know that's kind of awkward but it does work. It could probably be made a bit simpler and easier to read by referencing the types by name instead of by path. (I would just ask an LLM to add in the other possibly-empty fields as well.)
Next, you'd modify the type guard function above to have the signature:
export function isPossiblyBrokenChatCompletionChunk(obj: unknown): obj is PossiblyBrokenChatCompletionChunk {
// body of this function can remain the same
}Then you could add a function with this signature:
function fixPossiblyBrokenChatCompletionChunk(chunk: PossiblyBrokenChatCompletionChunk): OpenAI.ChatCompletionChunk {
// Implementation goes here
}Then up above you'd use:
if (isPossiblyBrokenChatCompletionChunk(data)) {
const fixedChunk = fixPossiblyBrokenChatCompletionChunk(data)
transformedLines.push(`data: ${JSON.stringify(fixedChunk)}`);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've refactored it so there's a function to check if it's a possible chat completion chunk and another function to fill in any missing fields.
sharon-wang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Fix empty role field | ||
| if (choice.delta.role === '') { | ||
| choice.delta.role = 'assistant'; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this was originally added for Snowflake, but it's possible other providers may omit this as well
| // Transform tools to be compatible with OpenAI-compatible providers | ||
| // Some providers don't support the 'strict' field in tool function definitions | ||
| if (requestBody.tools && Array.isArray(requestBody.tools)) { | ||
| log.debug(`[${providerName}] Request contains ${requestBody.tools.length} tools: ${requestBody.tools.map((t: any) => t.function?.name || t.name).join(', ')}`); | ||
| for (const tool of requestBody.tools) { | ||
| if (tool.function && tool.function.strict !== undefined) { | ||
| delete tool.function.strict; | ||
| log.debug(`[${providerName}] Removed 'strict' field from tool: ${tool.function.name}`); | ||
| } | ||
| } | ||
| log.trace(`[${providerName}] Tools payload: ${JSON.stringify(requestBody.tools)}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on consolidating the empty input schema handling that is currently in models.ts in AILanguageModel.provideLanguageModelChatResponse() with the handling here, so that all the tool modifications are in one place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe moving it to the utils.ts for generic handling? Although I removed it, there was some model specific handling. I'm good either way as long as models.ts doesn't grow too large as it is already a all-in-one kind of file.
I really hope that providers adhere to a standard API. This special handling feels like a side effect of providers not implementing the API.
| * @param obj The object to check | ||
| * @returns True if the object appears to be a ChatCompletionChunk (possibly malformed) | ||
| */ | ||
| export function isPossiblyBrokenChatCompletionChunk(obj: unknown): obj is PossiblyBrokenChatCompletionChunk { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this type guard isn't correct for the definition of PossiblyBrokenChatCompletionChunk above. With that interface definition, even an empty object, {}, could count as a PossiblyBrokenChatCompletionChunk.
But this function requires it to be an object and have:
id: stringchoices: Array<unknown>object: "chat.completion.chunk"
With this mismatch between the type and the type guard, the type guard could tell you something is not a PossiblyBrokenChatCompletionChunk when in fact it is, and that could be a source of bugs in the future.
The type and the type guard should match (or at least be much closer). I suggest updating the type to include essential things like the id, choices, and object -- whatever things we can reasonably assume that an OpenAI-compatible implementation would include.
| } | ||
| // Check if it's a possibly broken chunk and fix it | ||
| // Otherwise, keep the original line | ||
| if (isPossiblyBrokenChatCompletionChunk(data)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a follow on to the note above about the type and type guard mismatch: if isPossiblyBrokenChatCompletionChunk() actually matched the PossiblyBrokenChatCompletionChunk type, then it would always enter this if block and never go into the else. (Assuming that data is a JS object, as opposed to something like a string or null.)



Address #10818
This used Vercel's MCP server to assist in the migration to v5.
Release Notes
New Features
ai-sdkto v5Bug Fixes
QA Notes
I did some light testing with tool calling with various providers. The chat response handling is where things changed the most.